[WC-3630] Gallery: fix refresh interval having no effect - #2487

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml

CI checks: could not retrieve (permission denied) — author confirms lint and tests pass locally (tasks.md §3.2 ✓).

Findings

⚠️ Low — Existing test assertion refreshIntervalMs: 0 will silently pass for the wrong reason

File: src/model/containers/__tests__/createGalleryContainer.spec.ts line 59–63
Note: The pre-existing test "bind gallery config to the container" checks refreshIntervalMs: 0. After this fix that expectation still passes only because mockContainerProps() defaults refreshInterval: 0, which now correctly produces 0 * 1000 = 0. That means the assertion was true before (hard-coded 0) and remains true after (computed 0) — it doesn't prove the new derivation logic is exercised. The new dedicated test at line 66–71 covers this properly, but the old assertion is misleading; it reads as "the default is 0 because it should be" rather than "because 0 × 1000 = 0". Consider updating the comment or toMatchObject to make the intent clear, e.g. add a comment // refreshInterval default is 0, so ms is also 0.

⚠️ Low — showSilentRefresh threshold inconsistency not tracked

File: src/model/containers/Gallery.container.ts line 161
Note: The design doc notes that showSilentRefresh uses props.refreshInterval > 1 (exclusive), whereas the pre-migration code used >= 1 (inclusive). The change intentionally leaves this as-is ("separate, cosmetic difference"), but there is no follow-up ticket or TODO comment in the code. A user setting refreshInterval = 1 will have periodic refresh working (fixed by this PR) but no silent-refresh indicator (off-by-one vs. the original behaviour). Low severity since the design doc calls it out — just worth a TODO or a linked ticket to avoid the inconsistency being forgotten.

Positives

• The two-line fix is minimal and exactly mirrors how Datagrid 2 wires the same token — easy to verify correctness by inspection.
• Fake-timer tests cover both the positive case (reload fires at exactly 5000 ms, not 4999 ms) and the negative case (no reload for 60 s at interval 0), giving strong confidence the full DI path is exercised.
• The existing refreshInterval: 0 default in mockContainerProps means the rest of the test suite was already exercising the "no refresh" path implicitly, so no accidental regressions were introduced.
• OpenSpec artifacts (proposal, design, spec, tasks) are thorough and correctly scoped to the widget package.
• CHANGELOG entry is user-facing and describes the symptom, not the implementation.